Conversation
niklasf
force-pushed
the
refactor/effective-promoted
branch
from
c471450 to
82ad2c7
Compare
There was a problem hiding this comment.
Pull request overview
This PR centralizes “which promoted markers actually matter” behind a new Board._effective_promoted() hook, so core logic (king lookup, castling-rights handling, transposition keys, and default FEN rendering) can treat promotions consistently across variants.
Changes:
- Add
Board._effective_promoted()(defaulting toBB_EMPTY) and use it throughout king/castling/status/transposition-key logic. - Update
board_fen()sopromoted=Nonemeans “auto”: mark only_effective_promoted()pieces. - Add variant-specific
_effective_promoted()overrides (e.g., Suicide/Crazyhouse) and remove now-redundant variant overrides ofboard_fen()/_transposition_key().
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
chess/__init__.py |
Introduces _effective_promoted() in the base board and routes FEN/castling/king/status/hash logic through it. |
chess/variant.py |
Defines variant-specific _effective_promoted() behaviors and updates variant logic to use it instead of raw promoted. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if promoted is None: | ||
| promoted_mask = self._effective_promoted() | ||
| elif promoted: | ||
| promoted_mask = self.promoted | ||
| else: | ||
| promoted_mask = BB_EMPTY | ||
| if BB_SQUARES[square] & promoted_mask: |
There was a problem hiding this comment.
board_fen() recomputes promoted_mask (and may call _effective_promoted()) inside the per-square loop. Since promoted does not change while building the string, compute promoted_mask once before iterating squares to avoid repeated work in this hot path.
| promoted = self.has_chess960_castling_rights() | ||
| return super().board_fen(promoted=promoted) | ||
| def _effective_promoted(self) -> chess.Bitboard: | ||
| return self.kings & self.promoted if self.castling_rights else chess.BB_EMPTY |
There was a problem hiding this comment.
In SuicideBoard._effective_promoted(), gating on raw self.castling_rights can mark promoted kings as “effective” even when there are no valid castling rights (e.g. after parsing an invalid FEN where clean_castling_rights() would be empty). This can make fen()/epd() output and _transposition_key() depend on a castling-rights state that would otherwise be ignored. Consider basing the condition on castling rights that survive basic validation (for example, requiring a corresponding rook on the castling-rights square) so effective promotions track actually usable castling rights.
| return self.kings & self.promoted if self.castling_rights else chess.BB_EMPTY | |
| return self.kings & self.promoted if self.clean_castling_rights() else chess.BB_EMPTY |
1 participant
No description provided.